Emit pre-expansion feature gate warnings for negative impls and specialization#154527
Emit pre-expansion feature gate warnings for negative impls and specialization#154527fmease wants to merge 5 commits intorust-lang:mainfrom
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| &self, | ||
| negative_impls, | ||
| span.to(of_trait.trait_ref.path.span), | ||
| "negative trait bounds are not fully implemented; \ |
There was a problem hiding this comment.
negative impls != negative trait bounds (the Trait in impl Trait for () is not a trait bound, it's a trait ref)
tests/ui/auto-traits/ungated-impl.rs
Outdated
There was a problem hiding this comment.
Already covered by tests/ui/traits/negative-impls/feature-gate-negative_impls.rs. Likely a remnant of the time when negative impls and auto traits were part of the same feature, optin_builtin_traits.
| //~^ ERROR auto traits are experimental and possibly buggy | ||
|
|
||
| impl !AutoDummyTrait for DummyStruct {} | ||
| //~^ ERROR negative trait bounds are not fully implemented; use marker types for now |
There was a problem hiding this comment.
A gate test for negative impls doesn't belong in a gate test file for auto traits. Likely a remnant of the time when negative impls and auto traits were part of the same feature, optin_builtin_traits.
| if let Defaultness::Default(span) = defaultness { | ||
| self.psess.gated_spans.gate(sym::min_specialization, span); | ||
| self.psess.gated_spans.ungate_last(sym::specialization, span); | ||
| } |
There was a problem hiding this comment.
The idea is: Enabling feat specialization should imply min_specialization (the latter only supports the specialization of associated functions, not associated constants & types however). That's how the post-expansion feature gate already works (it visits the AST).
For the soft pre-expansion feature gate we basically want the same thing. However, I don't want to pass a param to the item parser that tells it if the item is meant to be free, associated or foreign. Instead, all 3 fn item kinds are now part of min_specialization from a pre-expansion perspective if they're marked default which is absolutely fine (default on free & foreign items gets rejected later on during AST validation).
We ungate specialization here since later in rustc_ast_passes::feature_gate, a span gated behind specialization requires feature(specialization) … which is undesirable for assoc fns ofc. Finally, in AST passes we allow feature(specialization) to unlock spans gated behind specialization and ones behind min_specialization. This leads to the desired semantics.
There was a problem hiding this comment.
Seems like some/all of this information should be a comment in the code?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Seems ok, though this stuff is way outside my wheelhouse and I wonder if someone who knows more about it should give a second review. I've given a few minor comments. The third commit is quite hard to review because there are things being moved around and also more substantive changes.
| = help: add `#![feature(auto_traits)]` to the crate attributes to enable | ||
| = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date | ||
|
|
||
| error[E0658]: negative trait bounds are not fully implemented; use marker types for now |
There was a problem hiding this comment.
Is the marker types suggestion no longer true? That seems like useful information.
There was a problem hiding this comment.
Readded as proper diagnostic note
| // For historical reasons, negative impls don't have a proper pre-expansion feature gate. | ||
| // We're now at least issuing a *warning* for those that only exist before macro expansion. | ||
| // FIXME(#154045): Turn their post-expansion feature gate into a proper pre-expansion one. | ||
| // As part of this, move these test cases into `feature-gate-negative_impls.rs`. |
There was a problem hiding this comment.
This comment confused me. Isn't this PR all about adding a proper pre-expansion feature gate? I guess it's not "proper" yet? I don't understand the difference.
There was a problem hiding this comment.
It's not "proper" yet as it's merely a warning for now (as backed by the MCP). Upgrading it to a hard error is the next step at some point in the future that's gonna be much harder to do (crater, fixing fallout downstream, T-lang nomination).
There was a problem hiding this comment.
Can you change "proper pre-expansion one" to "a pre-expansion hard error", or similar?
| if let Defaultness::Default(span) = defaultness { | ||
| self.psess.gated_spans.gate(sym::min_specialization, span); | ||
| self.psess.gated_spans.ungate_last(sym::specialization, span); | ||
| } |
There was a problem hiding this comment.
Seems like some/all of this information should be a comment in the code?
| /// The common case. | ||
| macro_rules! gate { | ||
| ($visitor:expr, $feature:ident, $span:expr, $explain:expr) => {{ | ||
| ($visitor:expr, $feature:ident, $span:expr, $explain:expr$(, $help:expr)?) => {{ |
There was a problem hiding this comment.
I understand the motivation, but I think a space before the $(, is much more common and better for readability -- it took me a minute to work out what was going on here.
Same on similar cases below.
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
It's probably too late but I've now split that commit and also rearranged the resulting commits. @rustbot review |
This comment has been minimized.
This comment has been minimized.
* utilize Kleene `?` two avoid having two macro matchers with almost
identical body
* inline `gate_legacy` since it only has one use and since it shouldn't
be used anywhere else anyway
* remove unnecessary explicit borrows of the visitor
* replace `if let Some(spans) = spans.get(…) { for span in …` with
`for &span in spans.get(…).into_iter().flatten()` to avoid rightward
drift and explicit dereferences
* rename `gate_all_legacy…` to `soft_gate_all_legacy…` since it's not a
"proper" gate that issues an error but merely one that emits a warning
View all comments
Follow up to #154475; part of #154045.
This shouldn't need any extra input from T-compiler or T-lang since it's legitimized by MCP 535.
However, I have a feeling that negative impls & specialization behind "
#[cfg(feature = "nightly")]" are more prevalent in the ecosystem compared to e.g., auto traits & box patterns, so these new warnings will probably hit a bunch of users. In any case, it's only a warning for now not an error so e.g., running crater "in deny mode" or nominating for a T-lang meeting discussion would be disproportionate (esp. since T-lang has confirmed in the past that this is a T-compiler matter).